stellar-contracts: unify access-grant expiry boundary semantics - #1281
Merged
llinsss merged 2 commits intoAug 31, 2026
Merged
Conversation
…ization paths Different code paths compared `now` against `expires_at` with different operators for what should be the same "is this expired" question: - check_access (AccessGrant) and get_active_consents (Consent): now >= expires_at - compact_storage's consent cleanup sweep: now > expires_at - is_emergency_authorized (EmergencyOverride): now <= expires_at (i.e. expired only once now > expires_at) At the exact expiry instant these paths disagreed: a consent record could be "expired" for get_active_consents but not yet stale enough for the cleanup sweep to remove; an emergency override could still authorize access one full second after check_access-style logic would deny an equivalent access grant. Adds one canonical `is_expired(now, expires_at) -> bool` helper (`now >= expires_at`, the fail-safe/conservative direction) and routes every authorization-relevant expiry check through it: AccessGrant (check_access, compact_storage sweep), Consent (get_active_consents, compact_storage sweep), EmergencyOverride (is_emergency_authorized), and decryption-delegation tokens (compact_storage sweep). No public ABI, storage layout, or error discriminants changed - only comparison logic. Closes DogStark#1159
|
@samuel2926i39-art Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Different authorization-relevant expiry checks in
stellar-contracts/src/lib.rscomparednowagainstexpires_atusing different operators for what should be one consistent question ("is this expired"):check_accessAccessGrantnow >= expires_atget_active_consentsConsentnow >= expires_atcompact_storage's consent cleanup sweepConsentnow > expires_atcompact_storage's access-grant cleanup sweepAccessGrantnow >= expires_atis_emergency_authorizedEmergencyOverridenow <= expires_atauthorized (i.e. expired only whennow > expires_at)compact_storage's decryption-delegation sweepnow >= expires_atAt the exact expiry instant (
now == expires_at) these disagreed: a consent record could already read as "expired" viaget_active_consentswhile the cleanup sweep didn't yet consider it stale enough to remove; an emergency responder's override could still authorize access for one full extra second past the point an equivalent access grant would already be denied bycheck_access.What changed
stellar-contracts/src/lib.rs(near the existingsafe_incrementcounter helper):pub(crate) fn is_expired(now: u64, expires_at: u64) -> bool { now >= expires_at }— the fail-safe/conservative direction (denies one instant earlier rather than later), and the convention the majority of existing call sites already used.check_access,get_active_consents, bothcompact_storagecleanup-sweep branches (consent and access-grant),is_emergency_authorized, and the decryption-delegation cleanup sweep.>→>=, now agrees withget_active_consents) andis_emergency_authorized(<=→ strict<, now agrees withcheck_access's convention for the equivalent access-grant case). Every other call site already used>=and is unchanged in behavior, just now expressed via the shared helper instead of an inline comparison, so a future edit can't silently drift it back out of sync.ContractErrordiscriminants changed — this is comparison-logic only.I could not compile, run, or test any of this.
stellar-contracts(the crate this file belongs to) currently fails to build on a cleanupstream/maincheckout with 346 pre-existing compile errors — duplicateconstdefinitions, a#[contracterror]macro panicking,ContractErrornot resolving inside someimplblocks, tests calling contract methods that don't exist (e.g.extend_access_grant, referenced by an already-#[ignore]d test intest_access_control.rs) — none of which are related to expiry logic or anything this PR touches. This is a much larger, unrelated problem with the base branch itself.Given that, everything here was done by careful direct code reading rather than compiler/test feedback:
AccessGrant/Consent/EmergencyOverride/delegation-tokenexpires_atfields are compared againstnowacross the whole file (grepped and manually read each call site) to build the table above.cargo build --libafter making the change and confirmed none of the 346 errors are newly introduced by or located at any line this PR touches (checked by exact line number and by searching the full error log foris_expired) — but this is not the same as a passing build, since the crate doesn't produce one either way.test_access_grant_expires_at_exact_boundary_instantintest_access_control.rs, modeled directly on the existing, already-passing-in-spirittest_access_expirytest's exact conventions) covering the boundary forcheck_access+compact_storageagreement, but I cannot confirm it actually compiles or passes given the crate-wide breakage.I'd strongly recommend a maintainer either fix the underlying compile breakage first, or manually build/test this specific diff against a working local checkout before merging — I don't want to claim green CI that doesn't exist. Happy to also take on the broader compile-breakage fix as its own effort if that's useful, though it looked substantial enough (346 errors) to warrant separate scoping.
Threat-model note
The behavior change makes both fixed paths more conservative, not less: consent records become eligible for cleanup one instant earlier than before (no data-integrity risk — cleanup just removes already-inactive/expired records), and emergency-override authorization now denies access at the same instant an equivalent access grant already would, closing a one-second window where two conceptually-identical "is this still authorized" checks could disagree. Neither change touches who can grant or revoke access — only exactly when an already-set expiry takes effect.
Closes #1159